feat: Add new hpa metrics to prevent prometheus timeseries duplication #2614
Conversation
…MetricSource and add tests
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: CountryTk The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: no
|
Welcome @CountryTk! |
k8s-ci-robot
added
size/L
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
|
Hey, could this please be reviewed @CatherineF-dev @rexagod |
|
Ok! In reviewing |
|
Overall LGTM. Two small comments. |
Thanks for the review, I've implemented your suggestions @CatherineF-dev |
|
Thank you for the patch. I believe this identifies a resource-agnostic pitfall where we loop over certain nested fields without including a primary key in the overall generated metrics' label-sets. I'll take a closer look tomorrow but so far this lgtm. |
|
Could you help enable I want to make a small change. Or could you apply these small changes? 96d948a |
For me it shows maintainer edit access is enabled. Anyway, I've added your suggested changes in the latest commit.
Nope because
|
| kube_horizontalpodautoscaler_spec_target_metric{horizontalpodautoscaler="hpa1",metric_name="events",metric_target_type="average",namespace="ns1"} 30 | ||
| kube_horizontalpodautoscaler_spec_target_metric{horizontalpodautoscaler="hpa1",metric_name="hits",metric_target_type="average",namespace="ns1"} 12 |
There was a problem hiding this comment.
These metrics are deleted. Could you double check?
There was a problem hiding this comment.
metric name "hits" and some others are now under their respective metric
hits is now under kube_horizontalpodautoscaler_spec_target_object_metric instead of kube_horizontalpodautoscaler_spec_target_metric
There was a problem hiding this comment.
This seems a breaking change. Could we keep kube_horizontalpodautoscaler_spec_target_metric not changed?
There was a problem hiding this comment.
Yes it is a breaking change but I was under the impression that it's fine since these metrics are experimental.
#2403 (comment)
I like the second option since the metric is still experimental so we can make changes to it. It will surely break some users but we didn't provide guarantees for this metrics. So as long as we make a smooth transition it should be fine.
The benefit of this approach is that the metrics will be specialized to the metric type they are targeting, so the labels will make sense for it and we won't run in scenarios where we have labels that are not used by some types.
There was a problem hiding this comment.
kube_horizontalpodautoscaler_spec_target_object_metric{target_name="",horizontalpodautoscaler="hpa1",metric_name="hits",metric_target_type="average",namespace="ns1"} 12
@dgrisonnet I'm not sure if this is what you meant in your comment (#2403 (comment)), can you PTAL?
There was a problem hiding this comment.
@CatherineF-dev Could I get some clarification on this task? Is it blocked or can we merge it?
There was a problem hiding this comment.
@rexagod yes that is similar to what I had in mind and we would do the same for container_resource_metrics, for resource_metrics, ... That said after reading my initial comment again, the idea of adding the fully qualified name of the object is wrong as it would make correlation more difficult.
I think we should have something like that:
kube_horizontalpodautoscaler_spec_target_object_metric{
object_kind=""
object_apiversion=""
object_name=""
horizontalpodautoscaler="hpa1",
metric_name="hits",
metric_target_type="average",
namespace="ns1"
}
kube_horizontalpodautoscaler_spec_target_container_resource_metric{
container_name=""
horizontalpodautoscaler="hpa1",
metric_name="hits",
metric_target_type="average",
namespace="ns1"
}
we could instead extend the existing metric and add target_name, target_kind and target_apiversion to it, but I feel like having dedicated metrics would be easier to extend and work with
There was a problem hiding this comment.
@dgrisonnet I'm a bit confused here, so do I delete the metrics I created and instead add target_name, target_kind and target_apiversion to the original metrics (kube_horizontalpodautoscaler_spec_target_metric and kube_horizontalpodautoscaler_status_target_metric) or do I leave it as it is right now because
having dedicated metrics would be easier to extend and work with
There was a problem hiding this comment.
what you did is in line with what I had in mind, I was just sharing more details about my reasoning with @rexagod
There was a problem hiding this comment.
@dgrisonnet it seems to me like this PR is in a limbo of some sort
Is there any chance this can get merged or
|
I'll bring this up in the call tomorrow. |
Hey, any updates? |
|
Is there any chance we can get this merged? |
|
@rexagod Any possibility we can get this merged? |
|
Hi @CountryTk, Looks like this PR does not fix external metric types. |
What this PR does / why we need it:
Added 4 new hpa metrics to prevent duplicated timeseries events like described in this issue: #2403
Added new metrics are:
How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
Cardinality is increased because of new metrics
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes # #2403
FYI: I've also tested this change in our prelive cluster and for us it fixed the issue